Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix subsetEquality: same referenced object on same level node of tree is regarded as circular reference #9322

Conversation

gejimayu
Copy link
Contributor

@gejimayu gejimayu commented Dec 18, 2019

Summary

This fixes #9089
This bug is introduced by this PR #8687
So basically, there is a flaw on the circular reference logic within a function called subsetEquality.
The logic is that, to avoid circular reference, it stores the references of nodes that it has been through within one singleton variable seenReferences. Whenever we have seen the same node (using the reference), we skip the subset equality checking (which is done recursively) and do deep equality checking instead.
The implication is that nodes within the same level of tree will use the same reference seenReferences. It's a problem because then it will cause an invalid handling on this case :

const referencedObj = {abc: 'def'};
const object = {
  a: {abc: 'def'},
  b: {abc: 'def', zzz: 'zzz'},
};
const thisIsNotCircular = {
  a: referencedObj,
  b: referencedObj,
};
expect(subsetEquality(object, thisIsNotCircular)).toBeTruthy(); // this test fails while it should succeed

On current logic, above case is considered circular referenced which isn't supposed to be. It will result in incorrect handling because circular reference does not check for subset equality and only check for plain equality instead. It makes sense for circular referenced object but not for normal object.

What we should do is to keep the seenReferences only for parent node and its children. The solution is quite simple, we only need to remove the reference immediately after the recursive function is done called for all of a node's children.

Test plan

I have added one test case using example above which would fail previously.
After the fix, now it succeeds.

@codecov-io
Copy link

codecov-io commented Dec 18, 2019

Codecov Report

Merging #9322 into master will increase coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9322      +/-   ##
==========================================
+ Coverage   64.76%   64.99%   +0.23%     
==========================================
  Files         280      278       -2     
  Lines       11985    11913      -72     
  Branches     2956     2935      -21     
==========================================
- Hits         7762     7743      -19     
+ Misses       3591     3539      -52     
+ Partials      632      631       -1
Impacted Files Coverage Δ
packages/expect/src/utils.ts 94.96% <100%> (-1.24%) ⬇️
packages/jest-transform/src/ScriptTransformer.ts 68.49% <0%> (-0.15%) ⬇️
packages/jest-config/src/index.ts 11.76% <0%> (ø) ⬆️
packages/jest-config/src/normalize.ts 77.74% <0%> (ø) ⬆️
e2e/v8-coverage/no-sourcemap/x.css
e2e/v8-coverage/no-sourcemap/Thing.js
packages/jest-runner/src/runTest.ts 2.43% <0%> (+0.24%) ⬆️
packages/jest-runtime/src/index.ts 67.34% <0%> (+1.9%) ⬆️
packages/jest-reporters/src/coverage_reporter.ts 61.83% <0%> (+10.61%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89c151b...cb4e2ea. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Dec 18, 2019

Thanks! This change seems reasonable to me, but I'm not too familiar with this part of the code base.

Mind updating the changelog as well?

@gejimayu gejimayu force-pushed the gianfranco.fh/fix-subsetEquality-circular-reference-logic branch from 54a05f5 to 7a27e77 Compare December 18, 2019 10:50
Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot!

@jeysal jeysal merged commit 30e08e9 into jestjs:master Dec 18, 2019
@gejimayu gejimayu deleted the gianfranco.fh/fix-subsetEquality-circular-reference-logic branch December 18, 2019 14:49
@gejimayu
Copy link
Contributor Author

gejimayu commented Dec 18, 2019

You're welcome. Thanks for a very swift response !
I'm really glad that i could help and contribute. Looking forward to contribute more ! This is my very first open source contribution by the way hahah

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reusing property matchers causes incorrect matching failure
5 participants